Use aligned_vec crate to eliminate unsound code #24
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #24 +/- ##
==========================================
+ Coverage 65.58% 66.52% +0.93%
==========================================
Files 4 4
Lines 985 923 -62
==========================================
- Hits 646 614 -32
+ Misses 339 309 -30 ☔ View full report in Codecov by Sentry. |
src/plane.rs
Outdated
| for v in pd.iter_mut() { | ||
| *v = T::cast_from(128); | ||
| Self { | ||
| data: avec_rt!([Self::DATA_ALIGNMENT]| T::cast_from(0); len), |
There was a problem hiding this comment.
This should probably not use avec_rt because it's technically a runtime-variable alignment. You could use AVec::<T, ConstAlign<Self::DATA_ALIGNMENT>> and manually create it. Bit less convenient but carries a compile-time alignment guarantee.
There was a problem hiding this comment.
we can use from_iter and keep in mind we want 128 as value.
There was a problem hiding this comment.
Yeah, my assumption was that because we needed the alignment to be different on wasm, we needed to use a runtime alignment here, but I suppose we could do that in a different way and still use const align.
There was a problem hiding this comment.
It is at compile time anyway, so it is a matter of using type A = ConstAlign<DATA_ALIGNMENT> and then use it where needed.
|
That crate has also an ABox, we can use into_boxed_slice, maybe it improves even better the situation. |
src/plane.rs
Outdated
| for v in pd.iter_mut() { | ||
| *v = T::cast_from(128); | ||
| Self { | ||
| data: avec_rt!([Self::DATA_ALIGNMENT]| T::cast_from(0); len), |
There was a problem hiding this comment.
we can use from_iter and keep in mind we want 128 as value.
|
I'd like to try at a manual implementation without the soundness bugs I've noticed. We could compare the two to see if there are performance differences because comparing to |
|
I think I have a working implementation here in my fork. I'll have to double-check some stuff, but it's probably good enough for some preliminary benchmarking. My notebook is probably not reliable enough for these sensitive benches (~5% differences could also be caused by CPU boosting etc., which is more pronounced on mobile platforms) |
Co-authored-by: Luca Barbato <[email protected]>
|
Yes, I'm also uncertain about the reliability of my benchmarks for the same reason. My original thought was that being less than 10% puts them within a range where noise could be the cause, given the very small scale of time that these benchmarks are running on. Though, I would personally prefer the version where we don't have to maintain any unsafe code, assuming performance is similar. |
|
I just meant that a laptop/mobile device isn't the best to do benchmarks on..
Generally agree, but unsafe code might buy us something here. E.g. the opportunity to (correctly and soundly) use uninitialized memory for better performance. Which is probably not possible (or a lot more complicated) with structs defined externally. Not 100% sure where I stand on this. |
|
We do use uninitialized vecs in a few other places in rav1e. I'm not a fan of it, but it does give performance improvements in some places--though in other places I've found it to give no benefit, likely due to compiler optimizations being used in some locations. |
|
I've tried for some time now, and I can't seem to get stable benchmark results even on my desktop machine. I think it's fine to just go ahead with this (over a manual implementation) without investigating this much further, but I also don't know how much of an impact this could have for consumers (esp. rav1e). Optimizations can still be done afterwards, and having reliably sound code seems more important. |
|
To improve the situation we'd need a MaybeFrame/MaybePlane I'm afraid. But that can be done later. I think. |
|
Based on the discussion, it sounds like we're good to go ahead and merge this. So I'll do so. |
| *v = T::cast_from(128); | ||
| Self { | ||
| data: AVec::from_iter( | ||
| Self::DATA_ALIGNMENT, |
There was a problem hiding this comment.
After skimming through aligned-vec, it's fine (and maybe preferable?) to just pass 0 here. That way there'd be no need for another target-dependent const variable.
This was created from the discussion in #23, to see how much performance impact we would have from swapping to a crate that provides an aligned Vec implementation. From the benchmarks within this crate, there seems to be some small amount of both positive and negative impact, depending on the benchmark at test. Change numbers below are compared to
mainbranch.